Respect generational files in recoveryDiff#55239
Respect generational files in recoveryDiff#55239DaveCTurner wants to merge 5 commits intoelastic:masterfrom
Conversation
Today `MetadataSnapshot#recoveryDiff` considers the `.liv` file as per-commit rather than per-segment and often transfers them during peer recoveries and snapshot restores. It also considers differences in `.fnm`, `.dvd` and `.dvm` files as indicating a difference in the whole segment, even though these files may be adjusted without changing the segment itself. This commit adjusts this logic to attach these generational files to the segments themselves, allowing Elasticsearch only to transfer them if they are genuinely needed. Closes elastic#55142 Resolves an outstanding `//NORELEASE` action related to elastic#50999.
DaveCTurner
added
>enhancement
:Distributed/Snapshot/Restore
|
Pinging @elastic/es-distributed (:Distributed/Store) |
| final List<StoreFileMetadata> perCommitStoreFiles = new ArrayList<>(); | ||
|
|
||
| for (StoreFileMetadata meta : this) { | ||
| if (IndexFileNames.OLD_SEGMENTS_GEN.equals(meta.name())) { // legacy |
There was a problem hiding this comment.
I think this is no longer relevant but looking for confirmation from someone who knows the history here.
| final Field.Store textFieldStored = random().nextBoolean() ? Field.Store.YES : Field.Store.NO; | ||
| doc.add(new TextField("body", textFieldContent, textFieldStored)); | ||
| final String docValueFieldContent = TestUtil.randomRealisticUnicodeString(random()); | ||
| doc.add(new BinaryDocValuesField("dv", new BytesRef(docValueFieldContent))); |
There was a problem hiding this comment.
This block of changes is mostly for extra logging, the one semantic change here is SortedDocValuesField becoming BinaryDocValuesField. I struggled to update the original SortedDocValuesField for some reason, I'm not too familiar with this API so maybe I missed something?
| equalTo(newCommitMetadata.size() - 4)); // segments_N, cfs, cfe, si for the new segment | ||
| assertThat(newCommitDiff.toString(), newCommitDiff.different.size(), equalTo(0)); // the del file must be different | ||
| assertThat(newCommitDiff.toString(), newCommitDiff.missing.size(), equalTo(4)); // segments_N,cfs, cfe, si for the new segment | ||
| assertTrue(newCommitDiff.toString(), newCommitDiff.identical.stream().anyMatch(m -> m.name().endsWith(".liv"))); |
There was a problem hiding this comment.
Now the .liv file is considered identical between the commits rather than different.
tlrx
left a comment
tlrx
left a comment
There was a problem hiding this comment.
LGTM
Thanks David, I appreciated the doc and comments. This looks good to me but a second review from someone who better knows this part of the code is necessary.
| } | ||
| // TODO NORELEASE no dummy docs since that includes deletes, yet we always copy the .liv file in peer recovery | ||
| indexRandom(true, false, indexRequestBuilders); | ||
| indexRandom(true, true, indexRequestBuilders); |
|
FYI I merged a lucene change that will make this easier down the road. It will be in Lucene 8.6 apache/lucene-solr#1434 |
ywelsch
removed
:Distributed/Snapshot/Restore
|
@s1monw do you think we should wait for Lucene 8.6 here? I think that'd be ok, and it'd make this more robust. |
|
@DaveCTurner the change in 8.6 will only work for segments that are changed by a Lucene 8.6 writer. I wonder if we need to do this for other segments too? |
|
Yes, we do need to handle older segments too, and I think what I've written here will be robust enough for them since we won't be writing any further segment generations without IDs. |
|
@DaveCTurner I'm wondering if we should update this PR and merge it? I can take care of it. |
|
Thanks @fcofdez please go ahead. Would be great to get this in. |
|
Closing this via #77695 |
12 participants
Today
MetadataSnapshot#recoveryDiffconsiders the.livfile as per-commitrather than per-segment and often transfers them during peer recoveries and
snapshot restores. It also considers differences in
.fnm,.dvdand.dvmfiles as indicating a difference in the whole segment, even though these files
may be adjusted without changing the segment itself.
This commit adjusts this logic to attach these generational files to the
segments themselves, allowing Elasticsearch only to transfer them if they are
genuinely needed.
Closes #55142
Resolves an outstanding
//NORELEASEaction related to #50999.